Add the planning module (updated PR) #592

danielabrozzoni · 2023-08-18T11:29:08Z

This PR builds on top of #481, fixing all the review comments.

I didn't squash my last commits on purpose to make review easier, I can squash them before merging if preferred.

tcharding

Looks good!

Code reveiw only, I'm not super knowledgeable on this code base.

ACK 4400f85

src/plan.rs

tcharding · 2023-09-07T20:19:32Z

Looks like this is waiting for review please @sanket1729 and/or @apoelstra

danielabrozzoni · 2023-09-12T14:29:45Z

d097867 - rebased and fixed review comments. CI is failing due to (I think) unrelated reasons, it's broken on master as well.

tcharding · 2023-09-12T14:46:38Z

#597 should fix CI

danielabrozzoni · 2023-09-12T18:53:15Z

Nice, rebased!

apoelstra · 2023-09-14T13:56:50Z

Can you run through each commit and make sure that cargo test passes?

danielabrozzoni · 2023-09-15T15:03:27Z

Can you run through each commit and make sure that cargo test passes?

Should be cleaner now, sorry!

CI is failing, but #598 should fix it.

The Satisfaction struct now contains `relative_timelock` and `absolute_timelock`, which represent the needed timelocks for that particular spending path. This is useful for the plan module. Co-authored-by: Daniela Brozzoni <danielabrozzoni@protonmail.com>

Add a `plan` module that contains utilities to calculate the cheapest spending path given an AssetProvider (that could keys, preimages, or timelocks). Adds a `get_plan` method on the various descriptor types. Co-authored-by: Daniela Brozzoni <danielabrozzoni@protonmail.com>

Co-authored-by: Alekos Filini <alekos.filini@gmail.com>

danielabrozzoni · 2023-09-18T08:22:38Z

a358076 - rebased on master

apoelstra · 2023-09-21T21:06:53Z

src/descriptor/mod.rs

+            Descriptor::Wsh(ref wsh) => wsh.plan_satisfaction(provider),
+            Descriptor::Sh(ref sh) => sh.plan_satisfaction(provider),
+            Descriptor::Tr(ref tr) => tr.plan_satisfaction(provider),
+        };


In d29c298:

In a followup PR, these are easy candidates to un-recursify (using iter.rs from #567 which was created long after this PR but merged long before).

For more context, what he means is what I'm doing in #594. The iter module implements a pretty hard core data structure, if you don't have the time or inclination to work it out right now just holla at me and I'll hack it up for you.

apoelstra · 2023-09-21T21:12:43Z

src/miniscript/satisfy.rs

+    ///
+    /// NOTE: If a descriptor mixes time-based and height-based timelocks, the implementation of
+    /// this method MUST only allow timelocks of either unit, but not both. Allowing both could cause
+    /// miniscript to construct an invalid witness.


In d29c298:

nit: I would suggest

NOTE: this method must ONLY allow time-based or height-based relative timelocks, except for the value zero which is valid as both a height or a time. Allowing both could cause miniscript to consruct an invalid witness for descriptors which mix height- and time-based timelocks. (We do not consider such descriptors to be "sane" but they can still be processed by this library, e.g. by using `parse_insane` to parse them.)

And same below (but change "relative" to "absolute").

But I'm not certain about the zero comment. Can somebody more familiar with timelocks check on that?

My main concern with the existing comment is that it doesn't make it clear that absolute and relative timelocks are considered separately; so you can have a height-based relative timelock and a time-based absolute timelock, for example.

Also, these comments are currently on Satisfier::check_older and Satisfier::check_after. Should they also be on the equivalent methods in AssetProvider?

apoelstra · 2023-09-21T21:19:54Z

src/plan.rs

+#[derive(Debug, Clone)]
+pub struct Plan {
+    /// This plan's witness template
+    pub(crate) template: Vec<Placeholder<DefiniteDescriptorKey>>,


In d29c298:

Maybe this was discussed in the other thread, but should we make this generic over Pk rather than using a DefiniteDescriptorKey?

apoelstra · 2023-09-21T21:28:04Z

a358076 looks amazing! I appreciate the detailed type-guided API with structs like TaprootCanSign.

I haven't tried to use this yet so there may be pain points but reading the code, this looks great. My comments are only nits.

apoelstra

ACK a358076

sanket1729

ACK a358076

I have quite a bit with this PR in Summer of bitcoin project with @Harshil-Jani who wrote some example programs using this API in #559. Thanks a lot for sticking with this PR for over a year :)

tcharding · 2023-09-22T02:54:13Z

Party! This one was open for a long time huh.

danielabrozzoni force-pushed the feat/planning-module branch 2 times, most recently from b0c2fd4 to 4400f85 Compare August 18, 2023 12:07

danielabrozzoni mentioned this pull request Aug 18, 2023

Add the "planning" module #481

Closed

tcharding approved these changes Aug 18, 2023

View reviewed changes

src/plan.rs Show resolved Hide resolved

src/plan.rs Outdated Show resolved Hide resolved

danielabrozzoni force-pushed the feat/planning-module branch from 4400f85 to d097867 Compare September 12, 2023 14:18

danielabrozzoni force-pushed the feat/planning-module branch from d097867 to 320c5ef Compare September 12, 2023 18:53

danielabrozzoni force-pushed the feat/planning-module branch from 320c5ef to 0bb4ba1 Compare September 15, 2023 13:42

afilini and others added 6 commits September 18, 2023 10:11

Add full_derivation_paths on DescriptorPublicKey

448fbd8

Fix test_cpp

fc20eb0

tests: plan capabilities

cb9a769

Co-authored-by: Alekos Filini <alekos.filini@gmail.com>

test: absolute/relative timelocks in satisfaction

a358076

Co-authored-by: Alekos Filini <alekos.filini@gmail.com>

danielabrozzoni force-pushed the feat/planning-module branch from 0bb4ba1 to a358076 Compare September 18, 2023 08:12

apoelstra reviewed Sep 21, 2023

View reviewed changes

apoelstra approved these changes Sep 21, 2023

View reviewed changes

sanket1729 approved these changes Sep 22, 2023

View reviewed changes

sanket1729 merged commit 780dbc6 into rust-bitcoin:master Sep 22, 2023
16 checks passed

notmandatory mentioned this pull request Oct 2, 2023

Socratic Seminar #12 Topics NashBitDevs/nashbitdevs.org#14

Closed

This was referenced Mar 7, 2024

Feature request: witness size estimator that understands key availability #55

Closed

Figure out how I can satisfy a descriptor without actually generating signatures #453

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the planning module (updated PR) #592

Add the planning module (updated PR) #592

danielabrozzoni commented Aug 18, 2023

tcharding left a comment

tcharding commented Sep 7, 2023

danielabrozzoni commented Sep 12, 2023

tcharding commented Sep 12, 2023

danielabrozzoni commented Sep 12, 2023

apoelstra commented Sep 14, 2023

danielabrozzoni commented Sep 15, 2023

danielabrozzoni commented Sep 18, 2023

apoelstra Sep 21, 2023

tcharding Sep 21, 2023

apoelstra Sep 21, 2023

apoelstra Sep 21, 2023

apoelstra Sep 21, 2023

apoelstra Sep 21, 2023

apoelstra commented Sep 21, 2023

apoelstra left a comment

sanket1729 left a comment

tcharding commented Sep 22, 2023

Add the planning module (updated PR) #592

Add the planning module (updated PR) #592

Conversation

danielabrozzoni commented Aug 18, 2023

tcharding left a comment

Choose a reason for hiding this comment

tcharding commented Sep 7, 2023

danielabrozzoni commented Sep 12, 2023

tcharding commented Sep 12, 2023

danielabrozzoni commented Sep 12, 2023

apoelstra commented Sep 14, 2023

danielabrozzoni commented Sep 15, 2023

danielabrozzoni commented Sep 18, 2023

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

tcharding Sep 21, 2023

Choose a reason for hiding this comment

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

apoelstra Sep 21, 2023

Choose a reason for hiding this comment

apoelstra commented Sep 21, 2023

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

tcharding commented Sep 22, 2023